-
Notifications
You must be signed in to change notification settings - Fork 402
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OpenStack: set correct hostnames for masters and workers #964
OpenStack: set correct hostnames for masters and workers #964
Conversation
|
||
decoded_data = json.loads(data) | ||
|
||
with open('/etc/hostname', 'w') as f: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add this to https://github.com/coreos/afterburn/ instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See e.g. coreos/afterburn#199 which landed exactly this for Azure. By contributing to afterburn, you're also contributing to Fedora CoreOS, and the functionality works early from the OS start.
Oh also, doing this from a static pod will run straight into openshift/origin#22826
EDIT: Nevermind on the last bit, that only applies to traffic exiting from non-hostNetwork pods but you wrote a systemd service.
In fact afterburn has OpenStack support today except...what's up with this change which calls the provider |
b41bba4
to
4bfe39b
Compare
@cgwalters indeed. As Openstack is not an homogeneous platform and the metadata component is only optional, the static Ignition platform ID is not useful here (i.e. it can't signal whether the metadata endpoint exists or not). |
To do that we'd have to have the MCO ship a drop-in for the systemd unit file that overrides (And for FCOS + OpenStack...people would use Ignition to write to |
An implicit thing in this discussion that we should probably make explicit is this PR is trying to unconditionally set the metadata for OpenStack, but you are implicitly arguing to not to do it an require users (of FCOS) to manually enable it. I am curious about more background behind your latter argument; it seems...easier...if we just try to reach the metadata service always, and require users of OpenStack-without-metadata (the unusual case, surely) to disable it. |
In this PR though, if metadata isn't available, then we'll have a failed systemd unit which...to my knowledge, nothing will react to or care about - something probably for the MCO to roll up in its status? Not sure. |
For what it's worth, this fixes the issue we're having with OpenStack. A worker's hostname and therefore node & machine name is now e.g. We need the Nova names and node names to match for the kubernetes cloud provider to work: https://kubernetes.io/docs/concepts/cluster-administration/cloud-providers/#node-name-4 |
@cgwalters Maybe I'm wrong, but as I see the metadata service must be enabled to deploy ocp anyway. We pass Ignition configs to MCO using metadata service:
So, I wouldn't worry about the absence of the metadata service. |
/test e2e-aws-upgrade |
/test e2e-aws |
4bfe39b
to
245dc9a
Compare
I have three high-level concerns with the PR in its current form:
|
Regarding your first point: presumably the change here would apply to the initial master nodes created by the installer as well. If that is the case, we will remove the installer code that sets If not, it would have to stay because it fixes the exact same issue on those nodes. To your points 2 and 3: getting the hostname from DHCP is simply not how OpenStack is expected to operate. Yes the A cluster of networked OpenStack cattle nodes is expected to read the cloud metadata and use the hostname values specified there. Which is what this pull request does. This is what every single OpenStack cloud image I'm aware of does. I have just launched two extra Nova servers (one CirrOS and one CentOS) in my deployed OCP cluster and their hostnames ended up being identical to their names in Nova. The hostname is initially set by the value from DHCP and it is then immediately overridden by the value from the metadata. It seems to me that we are using Afterburn exactly what it was designed for: to update the system's state by querying the cloud provider. And we do it to get the behaviour that the OpenStack users know and expect. I suppose the ultimate fix would be to run Afterburn (or something that does the same thing) in the OpenStack CoreOS images directly. I expect that will take longer and our cloud provider work will be blocked in the meantime. |
/test e2e-aws |
The unit files are arranged so that the dropin can be:
That's how CLCT handles it, when invoked with
I think the idea is that it's insecure to request metadata from a random link-local IP address unless we explicitly know that it's safe. cc @ajeddeloh |
Folks, let's get this straight. My original commit contained a service that executed a small script that queried the metadata, read the node name, and wrote to the file. Then @cgwalters told me about the Afterburn utility, which does just that. The only issue in this case is that Afterburn does not correctly identify the name of the provider (like "openstack", but it is actually called "openstack-metadata"). That's why I explicitly added "--provider openstack-metadata" to fix it. The Metadata service should be available because we actively use it in kubelet. If the administrator is worried about security, he can always switch to https. What other improvements can I make to this patch so that we can merge it? |
/approve |
Although, now that I look more closely this could just go in the |
/hold per above |
This commit sets hostnames equal to OpenStack node names.
245dc9a
to
76e932b
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgwalters, Fedosin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This commit sets hostnames equal to OpenStack node names.